-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup more ActionListener Delegation Spots #69662
Cleanup more ActionListener Delegation Spots #69662
Conversation
Cleaning up the remaining spots where the short-wrapper methods could be used that I could find.
Pinging @elastic/es-core-infra (Team:Core/Infra) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks fine, but I have a general comment about it and recent PRs around delegation.
The concept of delegation is good here. We often have listeners that need to be wrapped to append some response or failure logic. The problem I see is with the multitude of options now available. Looking at ActionListener now, there are quite a few abstract classes for dealing with delegation. Several examples in this PR use ActionListener.Delegating. That seems to be so that the delegate is a member that is available for use within onResponse. But I find this confusing because it's difficult to understand when one class or lambda method should be used over another.
I guess my general question is where is the end goal here? While the raw lines of code have been reduced, IMO the complexity has grown considerably in understanding which utilities are available for composing action listeners.
Thanks for taking a look + sorry for the delay here @rjernst I completely missed your review :(
The goal here isn't just in reducing LoC and cosmetics. It's mostly to get a tighter handler on what callbacks can throw where via assertions that were recently added like #69420 and to force certain patterns on these listeners. We had a number of bugs from not having these assertions where completely unexpected failures in these listeners would just be logged somewhere (sometimes even at debug) while also causing a memory leak.
That's a fair criticism. I guess I mostly go with the lambda version so long as there's not need to reference
++ I think ideally we should try and move to a world where things with clearly defined behavior like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation @original-brownbear. The vision and this PR LGTM.
@elasticmachine update branch |
Thanks Ryan! |
Cleaning up the remaining spots where the short-wrapper methods could be used that I could find.
Cleaning up some of the remaining spots where the short-wrapper methods
could be used.